Skip to content

chore(apollo_deployments): unify deployment functions #6363

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

Itay-Tsabary-Starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

reviewable-StarkWare commented May 7, 2025

This change is Reviewable

Copy link
Collaborator

@matanl-starkware matanl-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 5 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Itay-Tsabary-Starkware)


crates/apollo_deployments/src/deployment_definitions/sepolia_integration.rs line 23 at r2 (raw file):

    );

const FIRST_NODE_NAMESPACE: &str = "apollo-sepolia-integration-0";

Should you mention "sepolia" somewhere?
I find it confusing that in all modules it has the same name "FIRST_NODE_NAMESPACE".


crates/apollo_deployments/src/deployment.rs line 281 at r2 (raw file):

    namespace: &'static str,
) -> InstanceConfigOverride {
    let accepted_node_id_range = 1..=MAX_NODE_ID;

Seems like overkill.
What about checking that id >= 1 && id <= MAX_NODE_ID?


crates/apollo_deployments/src/deployment.rs line 299 at r2 (raw file):

    const MEMPOOL_SERVICE_PORT: u16 = 53200;

    if id == 1 {

Consider changing the id to be zero based, therefore compatible with "node_0" semantic.
In the secret_key you can encode "id+1" for simplicity.


crates/apollo_deployments/src/deployment.rs line 303 at r2 (raw file):

            "",
            true,
            "0x0101010101010101010101010101010101010101010101010101010101010101",

Consider extracting the secret_key formatting outside the if/else and using it in 4 places.

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware force-pushed the 05-07-chore_apollo_deployments_unify_deployment_functions branch from e7ba6f8 to cf21247 Compare May 7, 2025 13:10
@Itay-Tsabary-Starkware Itay-Tsabary-Starkware force-pushed the 05-07-chore_apollo_deployments_remove_consts_from_instance_config_overrides branch 2 times, most recently from 0e9e401 to 38d8e41 Compare May 7, 2025 13:28
@Itay-Tsabary-Starkware Itay-Tsabary-Starkware force-pushed the 05-07-chore_apollo_deployments_unify_deployment_functions branch from cf21247 to 223d97b Compare May 7, 2025 13:28
@Itay-Tsabary-Starkware Itay-Tsabary-Starkware force-pushed the 05-07-chore_apollo_deployments_remove_consts_from_instance_config_overrides branch from 38d8e41 to 7f295f6 Compare May 7, 2025 14:13
@Itay-Tsabary-Starkware Itay-Tsabary-Starkware changed the base branch from 05-07-chore_apollo_deployments_remove_consts_from_instance_config_overrides to graphite-base/6363 May 8, 2025 07:01
@Itay-Tsabary-Starkware Itay-Tsabary-Starkware force-pushed the 05-07-chore_apollo_deployments_unify_deployment_functions branch from 223d97b to 5908d8e Compare May 8, 2025 07:01
@Itay-Tsabary-Starkware Itay-Tsabary-Starkware changed the base branch from graphite-base/6363 to main May 8, 2025 07:01
Copy link
Contributor Author

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 5 files reviewed, 4 unresolved discussions (waiting on @matanl-starkware)


crates/apollo_deployments/src/deployment.rs line 281 at r2 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Seems like overkill.
What about checking that id >= 1 && id <= MAX_NODE_ID?

The code below

    assert!(id >= 1 && id < MAX_NODE_ID, "Node id {} is out of range [1,{:?})", id, MAX_NODE_ID);

Results in this error

manual \`RangeInclusive::contains\` implementation  
for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual\_range\_contains  
\`#\[warn(clippy::manual\_range\_contains)\]\` on by defaultclippy[Click for full compiler diagnostic](rust-analyzer-diagnostics-view:/diagnostic message [0]?0#file:///home/itay/workspace/sequencer3/crates/apollo_deployments/src/deployment.rs)

deployment.rs(281, 13): use: \`(1..=MAX\_NODE\_ID).contains(&id)\`

However, when transitioning to zero-based indexing as your comment below suggested, the condition itself becomes simpler as usize is automatically > 0.

TLDR: done.


crates/apollo_deployments/src/deployment.rs line 299 at r2 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Consider changing the id to be zero based, therefore compatible with "node_0" semantic.
In the secret_key you can encode "id+1" for simplicity.

Done.


crates/apollo_deployments/src/deployment_definitions/sepolia_integration.rs line 23 at r2 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Should you mention "sepolia" somewhere?
I find it confusing that in all modules it has the same name "FIRST_NODE_NAMESPACE".

They are all private consts, do you suggest renaming them in each respective module?

Copy link
Collaborator

@matanl-starkware matanl-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware)


crates/apollo_deployments/src/deployment.rs line 303 at r2 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Consider extracting the secret_key formatting outside the if/else and using it in 4 places.

Actually, what I meant was something like (avoid 2 times formatting):

Code snippet:

let secret_key = format!("0x.....{}", id + 1);
let validator_id = format!("0x{}", id + 1);
if (id == 0) {
    InstanceConfigOverride::new(
        &secret_key,
        ...
        &secret_key,
        validator_id,
    )
}
else {
    InstanceConfigOverride::new(
        &secret_key,
        ...
        &secret_key
        validator_id,
    )
}

crates/apollo_deployments/src/deployment_definitions/sepolia_integration.rs line 23 at r2 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

They are all private consts, do you suggest renaming them in each respective module?

If you accept my suggestion, then the answer is yes - all others should be renamed as well.
But I also see the "beauty" that they all have the same name.
I leave it for you to decide.

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware force-pushed the 05-07-chore_apollo_deployments_unify_deployment_functions branch from 5908d8e to c11c405 Compare May 8, 2025 08:32
Copy link
Contributor Author

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 5 files at r1, 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware)

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware added this pull request to the merge queue May 8, 2025
Merged via the queue into main with commit 4e93460 May 8, 2025
10 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants